feat(hooks): add MessageDisplay hook for mid-turn streaming - #6489
Conversation
Fires repeatedly as the assistant reply streams, before Stop (which only fires once at the end of the turn). Fire-and-forget, cumulative text payload, debounced (~200ms) except for the unconditional final firing. Fires from the single streaming loop in client.ts shared by the terminal UI and ACP paths. Fixes #6488
| } | ||
| } | ||
|
|
||
| // Final MessageDisplay flush: this turn.run() stream is exhausted, so this |
There was a problem hiding this comment.
[Critical] Three early return turn paths inside the for await loop (always-on loop detection ~line 2476, heuristic loop detection ~line 2507, stream error ~line 2571) bypass this final is_final: true flush. The finally block at ~line 2867 only handles memory prefetch/span cleanup — no MessageDisplay flush.
Hook scripts that rely on is_final: true to flush buffers (the documented contract: "a hook script knows to flush rather than wait for more text that will never arrive") will silently never receive the completion signal when the turn ends via loop detection or an API error.
Suggested fix: Move this flush into the existing finally block so it fires on all exit paths:
// In the finally block:
if (messageDisplayEnabled && !signal?.aborted) {
this.fireMessageDisplayHook(
messageBus, messageDisplayId,
messageDisplayState.displayedText, true, signal);
}— qwen3.7-max via Qwen Code /review
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
packages/core/src/core/client.ts:1238 |
Fire-and-forget messageBus.request() has no concurrency bound — slow hook commands can accumulate concurrent processes |
Track in-flight promise; skip or chain when previous request is still pending |
packages/core/src/core/client.ts:2576 |
Final flush unconditionally re-sends identical text when last debounced flush already carried full cumulative text | Route through stepMessageDisplay(state, '', now, debounce, true) or document the duplicate-firing contract |
packages/core/src/core/client.ts:2576 |
Final flush fires with empty displayed_text for tool-call-only turns |
Gate on messageDisplayState.displayedText !== '' |
packages/core/src/core/client.ts:2576 |
Final flush doesn't check signal.aborted (adjacent Stop hook does) |
Add && !signal.aborted to the guard |
packages/core/src/core/client.ts:1261 |
.catch error handler in fireMessageDisplayHook is untested |
Add test where messageBus.request rejects; assert debug logger warn |
packages/core/src/core/client.test.ts:7575 |
Mid-stream debounced flush path has no integration test (only final flush is exercised) | Add test with vi.useFakeTimers() advancing past debounce window mid-stream |
packages/core/src/core/message-display-buffer.ts:8 |
JSDoc for MESSAGE_DISPLAY_DEBOUNCE_MS references competitor's internal architecture |
Simplify to describe what the constant does without the comparison |
— qwen3.7-max via Qwen Code /review
- Chain fire-and-forget MessageDisplay requests per message_id instead of firing them fully unbounded, so a slow hook command can't pile up concurrent processes. - Gate the final flush on non-empty displayed_text and !signal.aborted, matching the adjacent Stop hook's guard. - Document why the final flush intentionally re-sends the last debounced text (is_final itself is new information). - Simplify the debounce constant's JSDoc to drop the competitor comparison. - Add tests for the mid-stream debounced flush and the rejected-request warn path.
…lay calls fireMessageDisplayHook now chains per-message_id through a promise (see previous commit), so the final flush's actual messageBus.request() call lands a few microtask ticks after the generator itself finishes — the mid-stream-flush test needs to let that chain settle before asserting.
|
Thanks for the thorough review, @wenshao! Addressed all seven points in 85e6f31:
Also merged the branch up to date with |
|
Thanks for the PR, @yanchenko! Template: headings deviate from Problem: Real gap. Issue #6488 clearly describes that no hook event fires during streaming — Direction: Aligned. Claude Code's CHANGELOG confirms Size: 342 production lines / 452 test lines / 103 schema lines across 18 files. Core paths touched ( Approach: One insertion point in the shared streaming loop ( Moving on to code review and testing. 🔍 中文说明感谢贡献,@yanchenko! 模板: 标题与 问题: 真实缺口。Issue #6488 清楚描述了流式输出过程中没有任何 hook 事件触发—— 方向: 对齐。Claude Code CHANGELOG 确认 规模: 342 行生产代码 / 452 行测试 / 103 行 schema,共 18 个文件。触及核心路径( 方案: 共享流式循环中单一插入点,纯防抖状态机( 进入代码审查和测试 🔍 — Qwen Code · qwen3.7-max |
2a. Code ReviewIndependent proposal: add Comparison: the PR matches and exceeds this. The pure Reuse check: no existing shared debounce utility that fits — Critical blockers: none found. Convention violations: none found. The new code follows existing patterns (enum values, interface shapes, MessageBus request/response, test structure). The prior automated review's 7 suggestions were all addressed in commit 85e6f31 — concurrency chaining, empty-text gate, signal.aborted check, error-path test, mid-stream debounce test, JSDoc cleanup, and duplicate-firing documentation. 2b. Real-Scenario TestingConfigured a Dev build (this PR)Hook output log (
|
|
This PR is a clean, well-scoped feature addition that does exactly what it promises. The problem is real — there's no way to observe a reply as it streams, and the The real-scenario test confirms the contract: three firings for a short reply, cumulative text, shared message ID, Every change in the diff is needed for the stated goal. No drive-by refactors, no scope creep, no speculative features. The test coverage is thorough (347 tests across 6 test files) without being excessive. The JSDoc is a bit more verbose than project convention, but for a new hook event with non-obvious firing semantics, that's defensible. If I had to maintain this in six months, I'd thank the author for extracting the pure state machine and writing tests that explain the debounce contract through assertions rather than prose. Approving. ✅ 中文说明这个 PR 是一个干净、范围合理的对等功能添加,完全实现了其承诺。 问题是真实的——目前没有方法在回复流式输出时观察内容, 真实场景测试确认了契约:短回复产生三次触发,累积文本,共享 message ID, diff 中每个改动都服务于目标。无顺手重构、无范围蔓延、无投机性功能。测试覆盖充分(6 个测试文件共 347 个测试)且不过度。JSDoc 比项目惯例略冗长,但对于一个触发语义不明显的新 hook 事件,这是合理的。 如果六个月后需要维护这个代码,我会感谢作者提取了纯状态机,并编写了通过断言而非散文解释防抖契约的测试。 批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM — clean feature addition, all 347 tests pass, real-scenario tmux test confirmed the MessageDisplay hook fires correctly with debounced cumulative text and immediate is_final. The prior review's 7 suggestions were all addressed. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Downgraded from Approve to Comment: CI still running.
The critical finding about early return turn paths bypassing the is_final: true flush is already covered by the existing review comment. One additional suggestion on test coverage below.
— qwen3.7-max via Qwen Code /review
| // to match the Stop hook's cancellation guard below. | ||
| if ( | ||
| messageDisplayEnabled && | ||
| messageDisplayState.displayedText !== '' && |
There was a problem hiding this comment.
[Suggestion] The !signal.aborted and displayedText !== '' guards on the final MessageDisplay flush are explicitly coded but have no corresponding test coverage. Two test cases would close the gap:
- Abort suppresses final flush: create an
AbortController, abort it before the stream ends (with MessageDisplay hooks enabled and some Content events already delivered), then assertmockMessageBus.requestwas never called withis_final: truefor the final flush. - Empty-text turn suppresses final flush: yield only non-Content events (e.g., a tool use event) from
mockTurnRunFnwith MessageDisplay hooks enabled, then assertmockMessageBus.requestwas never called witheventName: 'MessageDisplay'.
Both guards have clear intent in the source, but a regression would be silent — vacuous empty-text events or unnecessary hook spawns on cancelled streams.
— qwen3.7-max via Qwen Code /review
The three early `return turn` paths inside the streaming loop (always-on loop-detection safety, heuristic loop detection, and the stream Error event) exited before the final MessageDisplay flush, which only sat after the loop ended normally. Hook scripts relying on is_final: true to know when to flush never received it when a turn ended via loop detection or an API error. Extracts the flush into a shared closure and calls it from all four exits (the three early returns plus the normal fall-through), instead of only the one at the bottom of the loop. Adds regression tests for all three previously missed exits, plus the two guard-coverage tests requested in review (abort suppresses the flush, a tool-call-only turn with no Content events does not fire a vacuous empty-text event). Addresses the outstanding critical review comment and the follow-up test coverage suggestion on PR #6489.
|
Thanks for the follow-up, @wenshao and Qwen Code — both addressed in dd355fa:
Also added regression tests for the three previously-missed exit paths themselves (loop-detection x2, stream error), asserting the final flush now fires with the correct cumulative text on each. Full suite green (245/245 in |
| } | ||
| } | ||
|
|
||
| flushFinalMessageDisplay(); |
There was a problem hiding this comment.
[Suggestion] flushFinalMessageDisplay() is called from every exit out of the for await loop (all three early return turn paths + this post-loop call), but not from the finally block further down. The closure is declared inside the try block and is block-scoped, so it's inaccessible from finally.
If the async iterator throws an uncaught JS exception (transport-layer error, not a structured GeminiEventType.Error event), control jumps to finally and the is_final: true flush is skipped. Hook scripts relying on is_final as their completion signal would wait indefinitely.
To close this gap, the variables that flushFinalMessageDisplay captures (messageDisplayEnabled, messageDisplayState, messageDisplayId, messageBus) would need to be hoisted above the try block so the closure can be redeclared inside finally. The existing !signal.aborted guard inside the closure already suppresses it for abort-driven exits, so adding it to finally would only fire for genuine uncaught exceptions.
— qwen3.7-max via Qwen Code /review
| if (!messageBus) { | ||
| return; | ||
| } | ||
| const prior = this.messageDisplayChains.get(messageId) ?? Promise.resolve(); |
There was a problem hiding this comment.
[Suggestion] The promise-chain concurrency bound — which serializes hook requests per messageId so a slow hook process can't pile up concurrent instances — has no test that exercises its core invariant.
All existing MessageDisplay tests use mockResolvedValue({}) which resolves instantly, so the chain is never actually stressed. A test with a deferred promise on the first messageBus.request call would verify that the second call waits for the first to settle before dispatching. It would also be worth asserting that client['messageDisplayChains'].size === 0 after a turn completes, pinning that the .finally() cleanup works.
The debounce test ("fires a debounced mid-stream flush…") acknowledges the chain's existence in its comments but doesn't exercise it with a slow mock.
— qwen3.7-max via Qwen Code /review
| stopHookCount = stopResult.allOutputs.length; | ||
| break; | ||
| } | ||
| case 'MessageDisplay': { |
There was a problem hiding this comment.
[Suggestion] The case 'MessageDisplay': block that bridges messageBus.request() calls to hookSystem.fireMessageDisplayEvent() is not unit-tested. The client.test.ts tests exercise fireMessageDisplayHook → messageBus.request directly, bypassing this dispatch path entirely.
Other hook events in the same switch follow the identical pattern and also lack coverage here, so this is a broader gap. But since MessageDisplay is the only new event being added, a basic test exercising this dispatch path would be valuable — it would catch a regression if someone renamed the field or changed the extraction logic.
— qwen3.7-max via Qwen Code /review
DennisYu07
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
@qwen-code /resolve |
|
Qwen Code resolved the merge conflicts, but could not push to Merge Conflict Resolution Summary — PR #6489Conflicted file
What each side changed
ResolutionCombined both sides — neither change is contradictory:
No other files were modified. The commit message follows Conventional Commits format. |
Resolves conflict in docs/users/features/hooks.md: keeps the MessageDisplay event/matcher rows added by this branch alongside main's Tool id rename and new PermissionDenied event.
|
Merged main into this branch and resolved the docs/users/features/hooks.md conflict (combined main's Tool id rename + PermissionDenied row with this PR's MessageDisplay row). Pushed as c89a691 — full core test suite (253 tests) green, mergeable again. @wenshao — both rounds of your feedback were addressed in 85e6f31 and dd355fa; could you take another look / re-approve when you have a moment? The review decision is still showing as changes-requested from the first pass. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Downgraded from Approve to Comment: CI still running. Suggestion-level recommendations are in the Suggestion summary comment below.
— qwen3.7-max via Qwen Code /review
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
packages/core/src/hooks/types.ts:996 |
MessageDisplayOutput is exported but never imported anywhere. Dead export on the public type surface — contradicts the "fire-and-forget, no control effects" contract. |
Remove the interface. If output semantics are needed later, add them then. |
packages/cli/src/acp-integration/session/Session.test.ts:1957 |
Main ACP prompt loop is the only one of four streaming loops without an abort-suppression test for MessageDisplay. The other three loops (Stop-hook continuation, cron tick, background notification) each verify that is_final is suppressed on cancellation. |
Add a test that cancels the main prompt mid-stream and asserts no is_final: true MessageDisplay call. Model it on the background-notification abort test at ~line 2188. |
packages/cli/src/acp-integration/session/Session.test.ts |
No Session.ts test verifies that messageDisplay?.finish() fires in the finally block when the streaming loop exits via a thrown exception (as opposed to abort or normal completion). The client.ts suite covers this for GeminiEventType.Error, but Session.ts consumes sendMessageStream directly where errors surface as thrown exceptions. |
Add one test where sendMessageStream returns an async generator that throws after yielding a chunk, and assert that an is_final: true MessageDisplay call is made. |
packages/core/src/core/message-display-dispatcher.ts:229 |
Silent 5-second drain wait: drainWithTimeout() emits no log at the start of the wait — only a warning when the 5s timeout fires. An operator experiencing a slow hook sees the process stall with no indication of what is happening until the timeout warning appears. |
Emit a debug-level log at the start of the drain wait, e.g. debugLogger.debug("MessageDisplay: waiting for is_final delivery to settle"). This gives operators an immediate clue when the process stalls. |
packages/core/src/core/message-display-dispatcher.ts and message-display-buffer.ts |
No upper bound on displayed_text size. Cumulative text grows monotonically with no cap. For 100K+ char responses, each hook invocation receives the full text via stdin. While the coalescing design limits concurrent processes, the per-invocation payload is unbounded. |
Consider adding a configurable maximum size (e.g. 64KB). Past the cap, truncate from the head (keep the tail) or switch to delta mode. Document the cap in hooks.md. |
packages/core/src/core/message-display-dispatcher.ts:237 |
Drain timeout warning identifies the dispatcher by UUID messageId only — no hook command, count, or other identifying context. With multiple MessageDisplay hooks configured, the warning is not actionable without cross-referencing settings files. |
Enrich the warning with at minimum the hook count (e.g. "(N hooks configured)"), or have the caller wrap the warn callback to include the hook command name. |
packages/core/src/core/message-display-buffer.ts:11 and message-display-dispatcher.ts:34 |
MESSAGE_DISPLAY_DEBOUNCE_MS = 200 and MESSAGE_DISPLAY_DRAIN_TIMEOUT_MS = 5000 document WHAT they bound but not WHY those specific values were chosen. When a future maintainer needs to tune these, they have no basis for deciding. |
Add a // Rationale: line to each constant's JSDoc explaining the tradeoff that led to the chosen value. |
packages/core/src/core/message-display-buffer.ts:58 |
isFinal parameter of stepMessageDisplay is dead on the production path — the sole caller always passes false, and finish() bypasses this function entirely for the final flush. The parameter is exercised only in unit tests. |
Either remove isFinal from the production signature, or add a comment noting that the current dispatcher does not use it and the final flush is dispatched directly from finish(). |
— qwen3.7-max via Qwen Code /review
Local verification report — real TUI +
|
| Claim | Result |
|---|---|
Fires repeatedly mid-turn, before Stop |
✅ 11 mid-stream firings, all before Stop |
displayed_text is cumulative, not a delta |
✅ every payload is a strict prefix-extension of the previous |
| Debounced ~200 ms | ✅ firings at +0/199/399/601/801/1001/1201/1403/1604/1805/2005 ms |
Exactly one is_final: true, and it is last |
✅ per message_id |
Loop-detection early exit still flushes is_final |
✅ real content-loop trip (model.skipLoopDetection: false) → is_final fired, and Stop never fired at all on that path |
| Tool-call-only turn fires no vacuous empty event | ✅ 0 firings on the tool turn; the post-tool continuation got its own message_id |
| Abort suppresses the final flush | ✅ Esc mid-stream → 12 firings, no is_final |
Chained per message_id, never concurrent |
✅ 0 overlapping hook processes |
hasHooksForEvent fast-path gate |
✅ same settings.json on main → 0 firings, Stop still 1 |
Red/green on the dd355fa early-exit fix. With the three flushFinalMessageDisplay() calls at the early-return turn sites commented out, exactly the three regression tests you added go red (expected undefined to be defined); restored → green. The fix is real and the tests guard it.
× fires the final MessageDisplay flush when the always-on loop-detection safety trips mid-stream
× fires the final MessageDisplay flush when heuristic loop detection trips mid-stream
× fires the final MessageDisplay flush when the turn stream yields an Error event
Tests 3 failed | 5 passed
Unit suites all pass on the PR head: message-display-buffer.test.ts (8), hookEventHandler/hookSystem/hookPlanner/hookAggregator (345), client.test.ts -t MessageDisplay (8).
🔴 Finding 1 (blocking) — the ACP / IDE / qwen serve path never fires MessageDisplay
The PR description says:
Fires from the single
for awaitloop inclient.tsthat both the terminal UI and ACP paths already share, so this is one insertion point, not a per-surface reimplementation.
and docs/users/features/hooks.md adds:
Note: Fires in both the terminal UI and ACP (IDE/editor) sessions — they share the same underlying streaming event loop.
They do not share it. I instrumented both stream entry points in the same build and drove each surface once against the same settings.json:
- Terminal UI →
GeminiClient.sendMessageStream ENTER, 11MessageDisplayfirings, 1Stop. qwen serve→qwen --acpchild → onlyACP Session -> GeminiChat.sendMessageStream ENTER.GeminiClient.sendMessageStreamis never entered. 0MessageDisplayfirings — butStopstill fires.
Root cause: packages/cli/src/acp-integration/session/Session.ts:2401 consumes GeminiChat.sendMessageStream directly, and re-implements the Stop hook inline at Session.ts:2080 (gated at :2058). It never goes through the client.ts loop where this PR inserts the event. Session.ts contains zero references to MessageDisplay.
Worse, the daemon advertises the hook as live, so an IDE/daemon client is told it's active while nothing ever arrives:
$ curl -s localhost:41892/workspace/hooks
{"v":1,"workspaceCwd":"...","initialized":true,"disabled":false,
"hooks":[{"kind":"hook","eventName":"MessageDisplay","config":{"type":"command",...}}]}That's a direct consequence of adding MessageDisplay to IDLE_HOOK_EVENTS in packages/acp-bridge/src/status.ts without a corresponding fire site in the ACP session. Since #6488 explicitly names the IDE/ACP case as the gap being closed, this needs either a second insertion point in Session.ts or an honest scope reduction (and the doc note removed).
🔴 Finding 2 — a slow hook builds an unbounded, non-coalescing backlog
Chaining on messageDisplayChains bounds concurrency to one process per message_id, which I confirmed. But it does not bound queue depth. Mid-stream flushes are produced at most once per 200 ms while the chain drains at one per hook-duration — so whenever hook_duration > MESSAGE_DISPLAY_DEBOUNCE_MS, the backlog grows for the length of the stream.
With a 1200 ms hook against the same 2.4 s reply:
- The reply finished rendering at ~2400 ms.
is_final: truewas delivered at +12307 ms — 10.1 s after theStophook. - Every batch after the first carried text that was already stale on arrival (
text_len30, 35, 43 … while the full reply, 139 chars, had long since rendered). For the live-narration use case in feat: add MessageDisplay hook event for mid-turn streaming (CLI + ACP) #6488 that's the whole point of the event, and it narrates ten seconds behind.
Because displayed_text is cumulative, dropping a superseded queued batch is lossless. Suggestion: keep at most one pending payload per message_id and overwrite it with newer text while a hook is in flight (with is_final always winning), rather than prior.then(...) appending every batch. That preserves the ordering property the comment argues for, bounds the queue to O(1), and makes is_final land promptly.
🔴 Finding 3 — headless -p exits before the backlog drains, and is_final is lost
The docs state:
The final firing (
is_final: true) always fires immediately when the message ends, regardless of the debounce window, so the reply's tail is never dropped waiting on the debounce window.
The decision is immediate; the delivery is queued behind the backlog from Finding 2. In a headless -p run the process exits first and the tail is silently dropped. Same reply, same hook script, only the hook's duration varies:
| hook duration | MessageDisplay firings | is_final delivered? |
last text the hook saw |
|---|---|---|---|
| ~50 ms | 12 | ✅ yes | 139 / 139 chars |
| 300 ms | 8 | ❌ no | 104 / 139 chars |
| 1200 ms | 3 | ❌ no | 35 / 139 chars |
300 ms is an ordinary hook (a Python script, a curl to a TTS endpoint). A consumer that buffers until is_final never flushes, and never learns the message ended. Fixing Finding 2 mostly fixes this; awaiting the final flush (or draining the chain) before the turn returns would close it properly.
🟡 Minor / doc-accuracy
-
is_finalis not ordered beforeStop.flushFinalMessageDisplay()only schedulesprior.then(...)— a microtask — while theStoppath callsmessageBus.request(...)synchronously a few lines later, with noawaitin between. I observed both orders across runs (Stopat +2205 ms vs final at +2214 ms; and the reverse at +2211/+2217 ms), and Finding 2 turns it into a 10 s inversion. "Fires beforeStop" is true of the mid-stream firings only — worth saying so explicitly, since a hook author combining the two events will otherwise assume ordering. -
Cancellation delivers no terminal signal. The
!signal.abortedguard means Esc mid-stream produces firings and then simply stops — nois_finalever. Defensible, but it contradicts "always fires immediately when the message ends", and a buffering consumer hangs. Either document it, or fire a final event with anaborted/interruptedmarker. -
A tool-using turn produces multiple "final" messages per user turn. Verified: the tool-call turn fires nothing, the continuation gets a fresh
message_idwith its ownis_final: true. The PR body says this;docs/users/features/hooks.mddoes not. Hook authors will hit it immediately — please add it to the doc.
Repro
# mock LLM streams 24 chunks @100ms; hook logs its stdin payload with a ms timestamp
# settings.json: hooks.MessageDisplay -> {"type":"command","command":"node hook-log.mjs"}
# model.skipLoopDetection: false (heuristic loop detection is opt-in)
# terminal UI -> fires
node dist/cli.js # then send a prompt
# ACP / daemon -> does NOT fire
node dist/cli.js serve --port 41892 --workspace "$PWD"
curl -sX POST localhost:41892/session -d '{}' # -> sessionId, clientId
curl -sX POST "localhost:41892/session/$SID/prompt" \
-H "X-Qwen-Client-Id: $CID" \
-d '{"prompt":[{"type":"text","text":"hello"}]}'
curl -s localhost:41892/workspace/hooks # advertises MessageDisplay anyway
# Findings 2 & 3: make the hook sleep 300ms and re-run headless
MD_SLEEP_MS=300 node dist/cli.js -p 'hello'Two harness notes for anyone reproducing: the main baseline needs a real npm ci — symlinking the PR's node_modules makes esbuild inline the PR's packages/core through the workspace symlink and silently produces a contaminated "baseline". And timeout on a command hook is milliseconds (DEFAULT_HOOK_TIMEOUT = 60000), as the docs say — my first pass wrote "timeout": 30 and spent a cycle wondering why the hook got SIGTERM'd after 30 ms.
Overall: the buffer logic is clean and well-tested, the debounce/cumulative design is the right call, and the dd355fa early-exit fix is genuinely load-bearing (I broke it and your tests caught it). Finding 1 is what I'd want resolved before merge — either wire Session.ts, or scope the PR to the TUI and drop the ACP note plus the IDLE_HOOK_EVENTS entry so qwen serve stops advertising an event it never emits.
中文版(合并参考)
本地验证报告 — 真实 TUI + qwen serve,mock LLM,与 main 做 A/B
在 PR head(c89a69170)上用真实构建的 CLI(npm ci + npm run bundle)在 tmux 中端到端验证,配一个逐词流式返回的 mock OpenAI SSE 服务(24 个 chunk,每个间隔 100 ms),并用同样方式构建了 main 基线(271664b34)做对照。Hook 是真实的 command hook,把 stdin 收到的 payload 连同毫秒时间戳写入日志。
在终端 UI 这条路径上,流式语义与设计完全一致。 但 PR 描述里最核心的架构论断不成立,另有两处慢 hook 行为与文档承诺相矛盾。
✅ 已确认正确的部分
- 确实在回复还在书写时就触发:在 2.4 s 的流中于 1.45 s 截图,回复明显还没写完,此时已经派生了 6 个 hook 进程(见上方第一张图)。
displayed_text为累积文本而非增量:每次 payload 都严格是上一次的前缀扩展。- 防抖 ~200 ms 生效:触发时刻为 +0/199/399/601/801/1001/1201/1403/1604/1805/2005 ms。
- 每个
message_id恰好一次is_final: true,且一定是最后一次。 - 循环检测提前退出(真实触发内容重复循环,需
model.skipLoopDetection: false)仍然发出is_final,而该路径上Stop根本不会触发 —— 这正说明dd355fa那次修复的价值。 - 纯工具调用轮次不会发出空文本事件;工具执行后的续写轮次会拿到新的
message_id。 - Esc 取消时不发最终 flush。
- 同一
message_id的 hook 进程串行执行,无并发重叠。 hasHooksForEvent快速路径:同一份settings.json在main上触发 0 次,Stop仍为 1 次。
对 dd355fa 修复做了红/绿 A/B:把三个提前 return turn 处的 flushFinalMessageDisplay() 注释掉后,正是你新增的那三个回归测试变红(expected undefined to be defined),恢复后变绿。修复是真实有效的,测试也确实守住了它。
单元测试在 PR head 全部通过:message-display-buffer.test.ts(8)、hookEventHandler/hookSystem/hookPlanner/hookAggregator(345)、client.test.ts -t MessageDisplay(8)。
🔴 问题 1(阻塞合并)— ACP / IDE / qwen serve 路径根本不会触发 MessageDisplay
PR 描述称「从 client.ts 中终端 UI 与 ACP 共用的那个 for await 循环触发,因此只需一个插入点」,文档也写了「在终端 UI 和 ACP(IDE/编辑器)会话中都会触发 —— 它们共用同一个流式事件循环」。
实际并不共用。 我在同一份构建里同时给两个流式入口打了 trace,然后各驱动一次(配置完全相同):
- 终端 UI → 进入
GeminiClient.sendMessageStream,MessageDisplay触发 11 次,Stop1 次。 qwen serve→qwen --acp子进程 → 只有ACP Session -> GeminiChat.sendMessageStream,从未进入GeminiClient.sendMessageStream。MessageDisplay触发 0 次,而Stop照常触发。
根因:packages/cli/src/acp-integration/session/Session.ts:2401 直接消费 GeminiChat.sendMessageStream,并在 Session.ts:2080 自己内联重新实现了一遍 Stop hook(门控在 :2058),完全不经过本 PR 插入事件的 client.ts 循环。Session.ts 中对 MessageDisplay 的引用数为 0。
更麻烦的是,daemon 仍然对外宣告该 hook 已生效(GET /workspace/hooks 返回 "eventName":"MessageDisplay", "disabled":false),IDE/daemon 客户端会以为事件是活的,却永远收不到。这是在 packages/acp-bridge/src/status.ts 的 IDLE_HOOK_EVENTS 里加了条目、却没有在 ACP session 里加触发点的直接后果。鉴于 #6488 明确把 IDE/ACP 场景列为要解决的缺口,这里需要在 Session.ts 增加第二个插入点,或者老实收缩范围(同时删掉文档里的 ACP 说明)。
🔴 问题 2 — 慢 hook 会堆积出无上界、且不做合并的队列
messageDisplayChains 的链式串行确实把并发限制为每个 message_id 一个进程(我已验证)。但它没有限制队列深度。中途 flush 最快每 200 ms 产生一个,而队列的消费速度是每个 hook 时长一个 —— 只要 hook_duration > MESSAGE_DISPLAY_DEBOUNCE_MS,队列就会在整个流期间持续增长。
用 1200 ms 的 hook 跑同一段 2.4 s 回复:
- 回复约在 2400 ms 渲染完毕,而
is_final: true在 +12307 ms 才送达,比Stop晚了 10.1 秒。 - 第 2..11 批 payload 到达时携带的文本早已过期(
text_len依次为 30、35、43…,而完整回复 139 字符早就渲染完了)。对于 feat: add MessageDisplay hook event for mid-turn streaming (CLI + ACP) #6488 里「实时旁白」这个核心场景,等于慢了十秒。
由于 displayed_text 是累积的,丢弃被后续覆盖的排队批次是无损的。建议:每个 message_id 至多保留一个待发 payload,hook 在途时用更新的文本直接覆盖它(is_final 优先级最高),而不是用 prior.then(...) 把每一批都追加进去。这样既保留了代码注释里强调的顺序性,又把队列限制为 O(1),并让 is_final 及时送达。
🔴 问题 3 — headless -p 在队列排空前就退出,is_final 直接丢失
文档写着「最终触发(is_final: true)总是在消息结束时立即发出……因此回复的尾部绝不会被丢弃」。决策确实是立即的,但送达被压在问题 2 的积压队列后面。在 headless -p 运行中进程先退出了,尾部被静默丢弃。同一段回复、同一个 hook 脚本,只改 hook 的耗时:
| hook 耗时 | MessageDisplay 触发次数 | 是否收到 is_final |
hook 看到的最后文本 |
|---|---|---|---|
| ~50 ms | 12 | ✅ 是 | 139 / 139 字符 |
| 300 ms | 8 | ❌ 否 | 104 / 139 字符 |
| 1200 ms | 3 | ❌ 否 | 35 / 139 字符 |
300 ms 是很普通的 hook(一个 Python 脚本、一次 curl 打到 TTS 服务)。一个「攒够 is_final 再输出」的消费者将永远等不到 flush,也永远不知道消息已经结束。修好问题 2 基本就能缓解这一点;若要彻底解决,应在轮次返回前 await 最终 flush(或排空队列)。
🟡 次要 / 文档准确性
-
is_final并不保证早于Stop。flushFinalMessageDisplay()只是排了一个prior.then(...)微任务,而几行之后Stop是同步调用messageBus.request(...),中间没有await。我在不同运行中观察到两种顺序(Stop+2205 ms vs final +2214 ms;以及反过来的 +2211/+2217 ms),而问题 2 会把它放大成 10 秒级的倒挂。「在Stop之前触发」只对中途的那些触发成立 —— 建议在文档里讲清楚,否则同时用这两个事件的 hook 作者一定会踩坑。 -
取消时没有任何终结信号。
!signal.aborted这个门控意味着流式中途 Esc 之后只是「不再触发」,永远没有is_final。这个取舍本身合理,但与「消息结束时总会立即触发」的表述矛盾,且会让缓冲型消费者一直挂着。要么写进文档,要么发一个带aborted/interrupted标记的最终事件。 -
一次用户轮次里会出现多个「final」消息。 已验证:工具调用轮次不触发,工具之后的续写轮次拿到新的
message_id和它自己的is_final: true。PR 描述里提到了这点,但docs/users/features/hooks.md没有 —— hook 作者一上手就会遇到,建议补进文档。
关于验证环境的两个坑(供后续参考)
main基线必须做真正的npm ci:把 PR 的node_modules软链过去,esbuild 会顺着 workspace 软链把 PR 的packages/core内联进去,从而悄悄产出一个被污染的「基线」。- command hook 的
timeout单位是毫秒(DEFAULT_HOOK_TIMEOUT = 60000)而非秒;写"timeout": 30会在 30 ms 后 SIGTERM 掉 hook。
总体意见
buffer 逻辑干净、测试到位,防抖 + 累积文本的设计方向正确,dd355fa 的提前退出修复确实是承重的(我把它破坏掉后,你的测试立刻抓住了)。问题 1 是我希望在合并前解决的:要么把 Session.ts 也接上,要么把本 PR 范围收缩到 TUI,同时移除文档里的 ACP 说明和 IDLE_HOOK_EVENTS 条目,别让 qwen serve 继续宣告一个它永远不会发出的事件。
…, drain is_final before turn end Addresses the three findings from the local verification report on #6489: - ACP/qwen serve (Finding 1): the delivery logic now lives in a shared MessageDisplayDispatcher (packages/core), and Session.ts wires it into all four raw-stream loops (main prompt, Stop-hook continuation, cron tick, background notification) — these surfaces consume GeminiChat's stream directly and never enter GeminiClient.sendMessageStream, so they need their own fire sites. The daemon no longer advertises an event it never emits. - Slow-hook backlog (Finding 2): the per-message promise chain is replaced by coalescing delivery — at most one in-flight request plus one pending payload per message; newer flushes overwrite the pending slot, which is lossless because displayed_text is cumulative, and is_final is sticky. A slow hook now sees fewer, newer payloads instead of an ever-growing queue of stale ones. - Headless is_final drop (Finding 3): finish() resolves only once every enqueued payload has actually been delivered, and every exit out of the streaming loops awaits it (early returns, normal fall-through, and the enclosing finally for uncaught exceptions), so a short-lived -p process can no longer exit with the final payload still queued. As a consequence, is_final delivery now strictly precedes the Stop hook rather than racing it. Also: the failure log line carries the message_id, finish() is idempotent, the review-requested tests are added (mid-stream and final firings share one message_id; isFinal as the sole flush reason), and hooks.md gains a delivery-semantics contract covering coalescing, the drain guarantee, no is_final on cancellation, provisional displayed_text, and multiple messages per tool-using turn. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
…cancellation doc wording Adds MessageDisplay is_final coverage for the Stop-hook continuation loop, the in-session cron fire, and the background-notification loop, each with a normal-completion and an abort case. Adds three MessageDisplayDispatcher edge-case tests: a delivery settling just before the drain timeout, an abort arriving after a drain wait has already started, and addChunk called after abort but before finish(). Rewords the cancellation-timing doc bullet to state the actual criterion (abort signal state when finish() runs) rather than an approximation of it.
Ten more runs, three more defects, and a correction to the record. The record first. The runs that produced the evidence for the previous two commits, and for these, executed the review skill as it exists on main --- not this branch. main has no chunk plan, no territory agents, and no receipts, so any claim those commits made about which topology an agent ran under, or which chunk a symbol landed in, was reconstructed rather than observed. The defects they fix are real and were confirmed against main's own text, which this branch inherits unchanged: the severity taxonomy sits in Step 6 while Step 3 assigns severities, and cross-file impact analysis walks only the consumer direction. The causal stories about chunk agents were not observed and should not have been written as though they were. Now the new ones. The diff base. Agents were handed a diff command and left to choose a base. `main..HEAD` and `main...HEAD` differ by one character and by the entire meaning of the review: a two-dot diff against a main that has moved shows main's later commits reversed, so main's fixes read as the branch's regressions. A review of PR QwenLM#6626 approved the four files the PR actually changed, then warned the author publicly that their branch carried "typo regressions" in a file the PR never touched and should be rebased. main had corrected `compatability` to `compatibility` after the fork point. The branch had done nothing. Capture resolves the base once and hands agents a file; they never see a ref name, and a finding in a file outside the report's `files[]` is not a finding about this PR. The review body. "A Suggestion never goes in body" is stated twice and was violated anyway, because a model holding a finding it cannot anchor would rather say it somewhere than drop it. On PR QwenLM#6631 an unanchorable Suggestion about `session.ts:2048` --- a line in no hunk --- became a second paragraph of the public review body. So the rule stops being prose: for COMMENT the body is exactly one of three sentences plus the footer and nothing else, and you read what you are about to send and confirm it. A Suggestion that will not anchor is deleted; it is already in the terminal output and the Step 8 report. And the downgrade sentence. On PR QwenLM#6489 a review with three Suggestions and no Critical announced it had been "downgraded from Approve" --- telling the author the PR would otherwise have been approved, which was false: a Suggestion-only review is COMMENT on its own. Decide the event from the findings first, apply the downgrade flag second, and write the sentence only if it changed the answer.
Three defects, all found by reading what live reviews actually posted. The diff base. Agents were handed a diff command and left to choose a base. `main..HEAD` and `main...HEAD` differ by one character and by the entire meaning of the review: a two-dot diff against a main that has moved shows main's later commits reversed, so main's fixes read as the branch's regressions. A review of PR QwenLM#6626 approved the four files the PR actually changed, then warned the author publicly that their branch carried "typo regressions" in a file the PR never touched and should be rebased. main had corrected `compatability` to `compatibility` after the fork point. The branch had done nothing. Capture now resolves the base once and hands agents a file; they never see a ref name, and a finding in a file outside the report's `files[]` is not a finding about this PR. The review body. "A Suggestion never goes in body" is stated twice and was violated anyway, because a model holding a finding it cannot anchor would rather say it somewhere than drop it. On PR QwenLM#6631 an unanchorable Suggestion about `session.ts:2048` — a line in no hunk — became a second paragraph of the public review body. So the rule stops being prose: for COMMENT the body is exactly one of three sentences plus the footer and nothing else, and you read what you are about to send and confirm it. A Suggestion that will not anchor is deleted; it is already in the terminal output and the Step 8 report. The downgrade sentence. On PR QwenLM#6489 a review with three Suggestions and no Critical announced it had been "downgraded from Approve" — telling the author the PR would otherwise have been approved, which was false: a Suggestion-only review is COMMENT on its own. Decide the event from the findings first, apply the downgrade flag second, and write the sentence only if it changed the answer.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Downgraded from Approve to Comment: CI still running. Suggestion-level recommendations are in the Suggestion summary comment below.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /resolve |
|
Qwen Code resolved the merge conflicts, but could not push to Merge conflict resolution summary — PR #6489Conflicted files
|
Round-5 — re-verified
|
| committed test | + 100ms settle before asserting | |
|---|---|---|
| pristine dispatcher | 3 green | 3 green |
| abort guard deleted | only Stop-hook RED | all 3 RED |
…a cron fire is cancelled mid-stream and …a background notification response is cancelled mid-stream assert finals synchronously right after releaseCron!() / releaseNotification!() — before the loop's finally { await messageDisplay?.finish(); } has run. They'd pass whether or not is_final is suppressed.
The runtime is fine. I probed inside finish() during those exact tests:
finish() textLen=0 aborted=false <- the first, empty send
finish() textLen=19 aborted=true <- the cancelled cron stream
So the guard really is doing the suppressing; the tests just can't see it. Awaiting a macrotask before the assertion makes them real and produces no false positive on pristine code (column 2, row 1). The Stop-hook twin is already correct — it happens to await enough.
🟡 Finding B — clearTimeout(timer) is untested, and the test that claims to cover it doesn't
resolves the drain via the delivery settling just before the timeout, without warning advances to MESSAGE_DISPLAY_DRAIN_TIMEOUT_MS - 1 and stops, so the drain timer never fires. Its own comment says the drain "must resolve via delivery.finally clearing the timer, not via the timeout warning path" — but deleting clearTimeout(timer) leaves it green:
| committed test | + advance past the timeout | |
|---|---|---|
| pristine dispatcher | green | green |
clearTimeout deleted |
green ← vacuous | RED |
Worth covering because the regression is user-visible on the long-lived surfaces: an uncleared timer still fires and prints still running after 5000ms roughly five seconds after a turn that completed perfectly — the same class of spurious warning you deliberately suppressed for superseded mid-stream deliveries. Fix is await vi.advanceTimersByTimeAsync(10); before the two not.toHaveBeenCalled() assertions.
For balance: the other two new dispatcher tests are load-bearing. Making addChunk abort-aware reddens …mid-stream flush from addChunk called after abort, and letting an abort short-circuit the drain reddens …does not shorten an already-started drain wait. The addChunk-after-abort one is a nice honest characterization test.
⚠️ The branch conflicts with main again — and the conflict is inside a MessageDisplay region
mergeable: CONFLICTING. Eight commits behind. The conflict is in Session.ts's #executeCronPromptInner, in the same for (const part of candidate.content?.parts ?? []) loop:
main: if (!part.thought) finalRoundText += part.text;
this PR: if (!part.thought) { messageDisplay?.addChunk(part.text); }
Resolution is mechanical — keep both under the one !part.thought guard. (Session.test.ts conflicts too.)
The reassuring part: I simulated the careless resolution, deleting that single addChunk line, and the cron happy-path test you added in 1f005f0e9 goes red. The tests you just wrote protect the merge you're about to do. That's the coverage earning its keep on day one.
Recommendation
Approval stands — none of this is a product bug, and the runtime I verified in round 4 is unchanged. Findings A and B are test-quality: three tests currently assert something weaker than their names promise. Both fixes are one or two lines and I've verified each one flips its 2×2 correctly.
Merge main (carefully, in #executeCronPromptInner), fold in the two test fixes if you agree, and this is ready to land.
中文版(合并参考)
第五轮 —— 重新验证 1f005f0e9。两条 nit 均已修复,我的批准继续有效。新增的九个测试里有两个并没有在测它们声称要测的东西;另外分支现在与 main 在一处 MessageDisplay 代码区产生了冲突。
从 73add3d8e(我批准的那个)到 1f005f0e9,中间是一次 main 合并加上你的两个提交。我没有只看 diff 就相信,而是整体重新验证了一遍。
结论:没有任何运行时源码改动 —— message-display-dispatcher.ts 与我批准的版本逐字节相同(sha1 288d0d5e05fd),main 合并也没有增删任何一行 messageDisplay。两条 round-4 nit 都修好了,其中文档那条改得比我要求的更好。批准继续有效。下面两条是测试质量问题(不是产品 bug),外加一处值得花 30 秒小心处理的合并风险。
✅ 两条 nit 都已落地
两个改动的测试文件和 hooks.md 的 prettier --check 都干净了。取消语义那一句改得比我建议的更准确 —— 你写明了判据是「轮次结束那一刻 abort signal 的状态」,而不是「文本是否已经全部流完」。这正是 finish() 里 !this.signal.aborted 的实际行为,而这个微妙之处我当时并没有指出来。你自己把它挖出来了。
✅ 在重新构建的(合并后)产物上复验
Session.ts 四个 finish() 调用点仍全部位于 finally 中;client.ts 的那个仍早于 Stop。真实进程实测:headless 配 20s hook —— is_final 在 +0.03s(139 字符)→ Stop +5.04s → 进程 +5.09s 退出,stderr 一条告警。快 hook:12 次触发,+0.12s 退出。循环检测早退路径仍交付 is_final(510 字符)。测试套件:core 1324 通过,cli 459 通过;eslint、prettier 均干净。
并且 web-shell E2E Smoke 现在通过了 —— 这印证了 round-4 那个红是 base 落后导致的偏差,与我当时的判断一致。
🟡 问题 A —— 新增的三个「取消」测试里有两个是空转的
三个正常路径测试确实是承重的:分别删掉 #handleStopHookLoop、#executeCronPromptInner、#executeBackgroundNotificationPromptInner 里的 finish(),恰好只让对应那一个测试变红。你想补的覆盖缺口,确实补上了。
但如果把 finish() 里的 abort 守卫(!this.signal.aborted)删掉 —— 这会让每一个被取消的轮次都触发 is_final —— 三个取消测试里只有一个变红:
| 提交的测试 | 断言前加 100ms settle | |
|---|---|---|
| 原始 dispatcher | 3 绿 | 3 绿 |
| 删掉 abort 守卫 | 只有 Stop-hook 红 | 3 个全红 |
…a cron fire is cancelled mid-stream 和 …a background notification response is cancelled mid-stream 在 releaseCron!() / releaseNotification!() 之后同步就断言 finals,此时循环的 finally { await messageDisplay?.finish(); } 还没跑。所以无论 is_final 有没有被抑制,它们都会通过。
运行时是对的。 我在这两个测试运行期间往 finish() 里插了探针:
finish() textLen=0 aborted=false <- 第一次空的 send
finish() textLen=19 aborted=true <- 被取消的 cron 流
也就是说抑制 is_final 的确实是那个 abort 守卫,只是测试看不见它。在断言前 await 一个宏任务就能让它们变成真的测试,并且在原始代码上不会产生误报(第二列第一行)。Stop-hook 那个孪生测试本来就是对的 —— 它恰好 await 得够久。
🟡 问题 B —— clearTimeout(timer) 没有被任何测试覆盖,而声称覆盖它的那个测试并没有
resolves the drain via the delivery settling just before the timeout, without warning 把时间推进到 MESSAGE_DISPLAY_DRAIN_TIMEOUT_MS - 1 就停了,所以 drain 计时器根本没机会触发。它自己的注释写着 drain「必须通过 delivery.finally 清掉计时器来 resolve,而不是走超时告警路径」—— 但把 clearTimeout(timer) 删掉,它依然是绿的:
| 提交的测试 | 推进到超时之后 | |
|---|---|---|
| 原始 dispatcher | 绿 | 绿 |
删掉 clearTimeout |
绿 ← 空转 | 红 |
值得补,是因为这个回归在长生命周期路径上是用户可见的:没被清掉的计时器仍会触发,在一个本来正常完成的轮次结束约 5 秒后打印 still running after 5000ms —— 正是你专门为「被取代的中途投递」抑制掉的那类虚假告警。修法是在两个 not.toHaveBeenCalled() 断言之前加一行 await vi.advanceTimersByTimeAsync(10);。
作为对照:另外两个新增的 dispatcher 测试是承重的。把 addChunk 改成感知 abort,会让 …mid-stream flush from addChunk called after abort 变红;让 abort 短路 drain,会让 …does not shorten an already-started drain wait 变红。其中 addChunk-after-abort 那个是一个很诚实的行为刻画测试。
⚠️ 分支又与 main 冲突了 —— 而且冲突就在一处 MessageDisplay 代码区
mergeable: CONFLICTING,落后 8 个提交。冲突在 Session.ts 的 #executeCronPromptInner,位于同一个 for (const part of candidate.content?.parts ?? []) 循环里:
main: if (!part.thought) finalRoundText += part.text;
本 PR: if (!part.thought) { messageDisplay?.addChunk(part.text); }
解决方式是机械的 —— 在同一个 !part.thought 守卫下把两者都保留。(Session.test.ts 也有冲突。)
让人安心的是:我模拟了那种「直接取 main 侧」的粗糙解法,只删掉那一行 addChunk,结果你在 1f005f0e9 里新加的 cron 正常路径测试就变红了。你刚写的测试正好能保护你即将做的这次合并 —— 覆盖率上线第一天就发挥了作用。
建议
批准继续有效 —— 以上都不是产品 bug,我在第四轮验证过的运行时没有变化。问题 A 和 B 属于测试质量:有三个测试目前断言的东西比它们的名字承诺的要弱。两处修法都只需一两行,我已经逐一验证过它们能让各自的 2×2 正确翻转。
合并 main(在 #executeCronPromptInner 处小心处理),如果你认可就顺手带上这两处测试修复,然后就可以合了。
|
@qwen-code /resolve |
|
Qwen Code resolved the merge conflicts, but could not push to Merge Conflict Resolution Summary — PR #6489Branch: Conflicted Files1.
|
…QwenLM#6612) * feat(review): give every line of a large diff an accountable reviewer Review agents were handed the diff *command* and left to run it themselves. Shell tool output is capped at 30 000 characters and split head-1/5 / tail-4/5, so on a large changeset every agent received a few hundred lines off the top of the first file, the tail of the last file, and a truncation marker in place of everything between. Measured on a 211 000-character diff: 14.4% of the changeset, the same 14.4% for all ten agents. Nineteen of the twenty defects maintainers eventually confirmed on that PR lay in the hidden 85.6%. The ten-way dimension fan-out multiplied redundant reads of the visible sliver rather than adding coverage, and each review round sampled a different subset of the bugs depending on which files an agent happened to open on its own. The diff is now captured to a file and partitioned. `read_file` still caps a single read at ~25 000 characters, so writing the diff out is necessary but not sufficient — a whole-file read of that diff returns its first 611 lines. Chunks are therefore bounded by both a line budget (attention) and a character budget (what one un-truncated read returns), split on hunk boundaries, and never through the middle of a function. They tile the diff exactly, which is what makes the new coverage receipts checkable: past 500 diff lines each chunk gets one agent that owns it and must account for it, and a chunk with no receipt is re-reviewed before the run proceeds. "No blockers" can no longer be reported over code nobody read. Coverage alone did not close the gap. Chunk agents held every state-machine defect in that PR inside their assigned territory and reported none of them: the bugs were not inside any hunk but between new lines sitting two thousand lines apart, and what the agents lacked was not the lines but the question. A heavily rewritten file now also gets three whole-file agents that walk a fixed invariant checklist — mutable fields cleared on every exit path, timers cancelled on every close without discarding captured data, map inserts matched by deletes, retry counters incremented at every entry, status returns actually checked, error codes classified permanent versus transient, config honoured on every path, early returns that skip a required side effect. The checklist is split three ways deliberately: one agent asked to run all eight checks over a 2 400-line file runs one of them properly. Verification is sharded at eight findings per agent, because one verifier re-reading code for sixty findings degrades on the tail of its list. A verifier may now downgrade a Critical but never delete one — a rejected Critical is invisible to every later stage, a downgraded one still reaches a human. The reverse audit fans out per chunk instead of asking a single context-starved agent to re-read the whole diff, no longer skips verification, and stops after two consecutive dry rounds rather than one: on the PR that motivated this, the review reported "no blockers" twice and the next round surfaced five Criticals, three of them in code present since the first commit. * fix(review): keep small-diff reads inside the read_file cap Step 3A told every agent to read the whole diff in one call. `read_file` truncates a single call at ~25 000 characters, so a 500-line diff of long lines would come back short — the same blind spot the chunk plan removes, reintroduced at a smaller scale. Across the last 39 merged PRs that take the Step 3A path the largest diff is 23 570 characters, so this never fired in practice, but the margin is six percent. Step 3A now walks the chunk ranges, which are sized to fit one un-truncated read: one or two calls at this size. Derive a file's pre-change line count from the diff instead of measuring it with a second `git show` per file. `git show <base>:<newpath>` returns nothing for a renamed file, reporting zero pre-change lines and classifying a wholesale rewrite as light. The identity holds exactly for creations, deletions, renames and ordinary edits, and halves the process spawns. * fix(review): choose the topology from source lines, not diff lines Diff size is a bad proxy for review risk because test code dominates it. Across this repo's last 40 merged PRs the median diff is 41% test code and 14 of the 40 are more than half tests; PR QwenLM#6457, which motivated the territory fan-out, is itself 58% tests. Gating on raw diff lines therefore carved small production changes into territories: a change of 173 source lines shipping 489 lines of new tests went to the chunked topology, where its production code ended up owned by a single agent, when the dimension fan-out would have read it through eight lenses. Territory fan-out is worth it when there is a lot of risky code to divide, not a lot of lines. The gate is now `srcDiffLines > 500`, with `diffLines > 2400` as a second clause — a delivery bound rather than a risk one, since past that point chunking uses fewer agents than the ten-lens topology anyway and reading a diff that large dilutes all ten. On the 40-PR sample six PRs move back to the dimension fan-out, for about 5% more agents in total across the sample. Paths are classified as source, test, or generated, and the per-kind line counts ship in the fetch report. Chunking is unchanged: the plan still tiles every line, tests and generated files included. What the gate decides is how many reviewers there are and what each is asked to do. Heaviness is likewise restricted to source files — the invariant checklist asks about fields, timers, collections, and error taxonomies, and a rewritten test file has none of those. * fix(review): decode C-quoted diff paths as bytes `git diff` C-quotes any path with a control character or a non-ASCII byte, so a file named `sub/中文文件.ts` arrives as `"b/sub/\344\270\255..."`. The chunk planner stripped the backslashes, turning it into `sub/344270255...ts` — a name that exists nowhere. Every downstream use of the path then failed silently: the line count came back zero, the file could never be classified as heavy, and the chunk agent was told it was reviewing a file that does not exist. Reuse core's `unquoteCStylePath`, which reassembles the octal escapes as UTF-8 bytes, rather than keeping a second, wrong decoder here. Coverage was never affected — line ranges stayed correct — but this repo has non-ASCII paths, so the mislabelling was reachable. Also correct two places that claimed hunks are never split. They are: a hunk larger than the chunk target is split at a top-level declaration, because a brand-new file arrives as one enormous hunk and treating it as atomic would hand a single agent a 50 000-character territory. * fix(review): make diff capture and header parsing robust to git config Four defects, all found in review of this branch. Diff capture obeyed whatever the user's git config said. With `color.diff=always` every `diff --git` line arrives wrapped in ANSI escapes, the parser recognises none of them, and the plan comes back with zero files and zero chunks — the coverage guarantee silently evaluates to nothing. `diff.mnemonicPrefix` renames the `a/`/`b/` prefixes to `i/`/`w/` and every path is then wrong; `diff.external` and textconv filters emit output that is not a unified diff at all. Capture now pins `--no-ext-diff --no-textconv --no-color --unified=3` and the two prefixes. The `diff --git` header was split with a greedy regex. Git separates the two paths with a space and does not quote a path merely for containing one, so `a/img with space.png b/img with space.png` split into `space.png`. Usually the `---`/`+++` headers disambiguate, but a binary or mode-only section has neither. For a non-rename both paths are the same string, so the split point is arithmetic; a rename states its new path outright in `rename to`. A chunk boundary could land on a `-` line. Those exist only on the old side, so the "starts at a top-level declaration" guarantee did not hold for the post-change file an invariant agent later reads. Split points are now restricted to lines present on the new side. An `oversized` chunk — one hunk with no safe interior boundary — can exceed what a single `read_file` returns. Chunks now carry their character count, and a chunk agent is told to page when a read reports truncation. A `Covered:` receipt for a range the agent only half read is worse than no receipt at all. * fix(review): split past a distant boundary, and stop probing GitHub for anchors Both defects surfaced running the new review against PR QwenLM#6591. A 1431-line React component was emitted as a single 45 675-character chunk — nearly twice what one `read_file` returns — because the splitter looked for a safe boundary only inside the 400-line budget window, found none, and gave up on the entire remainder. Twenty-seven boundaries existed further along; the first sat 460 lines in. It now reaches past the window for the next one, so a single distant boundary can no longer collapse a whole file into one chunk. That PR goes from 15 chunks with one over the read cap to 18 with none. Step 7 validated comment anchors by trial. GitHub rejects an entire review with a 422 if any comment's line falls outside every hunk of its file, and the skill offered no cheap way to check, so a run against a real PR submitted five throwaway reviews carrying the bodies `Test`, `Test`, `t`, `t`, `t` to discover which anchors would stick. Those are permanent, public reviews on someone else's pull request. The fetch report now carries each file's hunks as new-side line ranges, which turns the check into a lookup, and the skill states plainly that a review is never submitted to test an anchor. * fix(review): stop reading hunk payload as metadata, and harden the plan Eleven defects from review of this branch. The worst two were silent. A unified diff emits a removed line whose content starts with `-- ` as `--- ...`, and an added line whose content starts with `++ ` as `+++ ...`. SQL, Lua and Haskell comments start with `-- `. The parser read those payload lines as file headers: the path was overwritten by the line's text, and the line vanished from the add/remove counts. A two-file diff — one SQL file losing a comment, one text file gaining a `++ ` line — came back with the second file named `plus line`. Metadata is now only recognised before a file's first hunk. The tiling invariant — every diff line belongs to exactly one chunk, which is what makes a missing coverage receipt mean something — was asserted only in tests. `buildDiffPlan` now checks it and refuses to return a plan with a hole. The rest: a split point could take a *deleted* blank line as evidence of the blank line before a declaration, though that blank exists only in the old file; whole-file invariant agents were pointed at `chunks[].files[]`, which merges hunks at lines 10 and 900 into one `10-902` span and would have had them report pre-existing defects as new; pure-deletion hunks were exported as the inclusive range `[N, N]`, so a right-side comment could be anchored where GitHub has no line and the 422 would sink the whole review; a deleted file could be marked heavy and send three agents to read a post-image that does not exist; a chunk holding a single line longer than one `read_file` can never be fully read by paging, and must now report itself uncoverable rather than receipt a lie; capture did not pin rename detection or `--no-relative`; `gitRaw` had no timeout, so a credential prompt on headless CI would hang forever; a failed base fetch was swallowed, leaving a stale merge-base and a structurally complete report describing the wrong diff; and local reviews still captured with a bare `git diff`, which `color.diff=always` alone renders unparseable. Adds an integration test that drives the real capture against a real repository under hostile git config, covering the paths synthetic fixtures cannot: renames and binaries and mode-only changes with spaces in their names, C-quoted non-ASCII names, and payload lines that impersonate headers. * fix(review): pin submodule output, and separate written lines from hunk spans Four defects from review of this branch. Diff capture left submodules to user config. `diff.ignoreSubmodules=all` hides a changed gitlink completely — a silent coverage hole in the file that is now the review's source of truth — and `diff.submodule=log` replaces the whole `diff --git` section with prose no parser can read. Both are pinned now, and the integration test asserts a bumped gitlink survives them. Whole-file invariant agents were handed `files[].hunks[]` as "the changed lines". A hunk spans the three context lines git prints either side of every change: on PR QwenLM#6457's `QQChannel.ts` those spans cover 1 962 new-side lines of which only 1 403 were written. The agent would have reported defects in 559 lines that predate the PR. The report now also carries `addedRanges[]` — the exact lines the change wrote — and the skill gates invariant agents on those, keeping `hunks[]` for the one thing it is right for, GitHub anchor validation. `Uncoverable:` was introduced as a chunk agent's answer for a chunk holding a line longer than one read, but the receipt accounting still demanded a `Covered:` line from every chunk and relaunched any chunk lacking one — so an uncoverable chunk would have been retried forever. It is now a first-class terminal status: accepted by the accounting, carried into Step 6 under "Not reviewed", and it blocks an Approve verdict. Step 3A, which also walks the chunk plan, is covered by the same rule. The integration test built its fixture repository inside the developer's git environment, so a global `core.hooksPath` or `commit.gpgsign` ran during the test and `~/.gitconfig` decided what the "clean" baseline was. It now disables system and global config, hooks and signing, and sets the executable bit through the index rather than shelling out to `chmod`, which does nothing on Windows. * feat(review): plan any captured diff, and stop the report outgrowing one read Seven items from review of this branch. None blocking; two of them were the skill promising a topology it could not deliver. Step 3B's chunk agents are "one per entry in `chunks[]`", and only `fetch-pr` produced a chunk plan. A local-diff review, and a cross-repo review in lightweight mode, therefore routed into the territory fan-out with no chunk list, no receipts and no tiling guarantee. `qwen review plan-diff <diff-file>` now emits the same plan from any captured diff; redirecting `git diff` or `gh pr diff` to a file already sidesteps the shell's character cap, so all four review paths share one mechanism. A bare diff has no tree to read a post-image from, so it gets chunk agents but no invariant agents, and says so by omission. The fetch report is read with the same `read_file` that truncates at 25 000 characters — and for a seven-file PR it was already 28 056. The tail of `chunks[]` was being silently lost: the coverage hole this design closes, reappearing one level up. `addedRanges[]` now ships only on `heavy` files, its only consumer, which brings that report to 24 992; the skill says to page the read; and the command prints a note when the report exceeds one read. It stays pretty-printed on purpose — a compact one-line JSON cannot be paged by line. The tiling assertion threw inside `fetch-pr` after the worktree existed and before any report was written, so an unforeseen diff shape killed the review outright. It now degrades to the documented diff-less report with a loud warning, keeping both the loudness and the review. `gitOpt` and `git` had no timeout, and `resolveMergeBase` uses `gitOpt` for a network fetch — the exact path whose credential prompt the `gitRaw` timeout was added to survive. All three wrappers now share a deadline and `GIT_TERMINAL_PROMPT=0`. Markdown under `docs/` or at the repository root classifies as `docs` and stays out of `srcDiffLines`, so a translation PR does not trip the territory gate. Markdown inside a source tree stays `source` — the bundled skill prompts are behaviour, not prose. Also: the user docs stated the gate without its `diffLines > 2400` clause, and `READ_FILE_CHAR_CAP` was exported but never used. It now backs the report-size warning. * test(review): unit-test the merge-base and plan-report seams The last open review thread asked for `resolveMergeBase`, `fileMetrics` and `gitRaw` to be testable with git mocked out. Three of the four functions it named have since moved: `classifyHeavy` is a pure function with unit tests, `fileMetrics` became `buildPlanReport`, which already takes an injected post-image resolver, and `gitRaw`'s output path is exercised by the real-git integration test. `resolveMergeBase` was still private and untested. It now lives behind a three-method `GitProbe` — fetch, refExists, mergeBase — that `fetch-pr` fills from the real wrappers. Seven tests cover the branches that matter and that no end-to-end run reaches: the tracking ref preferred over the local branch, the fall-through when the tracking ref shares no history, and above all the dangerous one — a failed fetch that still resolves a merge-base from a stale local ref, which produces a structurally complete report describing a diff nobody wrote. `buildPlanReport` gains seven of its own: the injected resolver is asked once per file and never for a binary, a null resolver means "no tree, decide nothing" rather than a guess, `addedRanges` ship only where an invariant agent will read them, and a pure-deletion hunk never reaches the anchorable ranges. * fix(review): see deletions, survive suppressBlankEmpty, and stop approving unread code Seven findings from review of the merged head. Three of them were the design contradicting itself. `diff.suppressBlankEmpty` prints a blank context line as a physically empty record rather than a lone space, and there is no command-line flag to override it — only `-c`. The parser advanced its new-side cursor for space-prefixed context alone, so every `addedRanges` entry after the first blank line shifted up by one, and the split-point heuristic stopped recognising blank lines. The capture now pins the config, and the parser treats an empty hunk-body record as context regardless, because a diff from `gh pr diff` or a hand-captured file never passes through that pin. A whole-file invariant agent was given the post-change file and the ranges the PR wrote. A deletion appears in neither. Removing a `clearTimeout()`, a `Map.delete()`, or a retry-counter increment is exactly what the checklist hunts, and the text it was handed cannot show a line that is no longer there — telling it to "cite the surrounding hunk" pointed at data it never received. Heavy files now carry a `diffRange` into the report, and the agent reads its own slice of the diff, where the `-` lines are. The receipt accounting demanded exactly one per chunk and said it applied to Step 3A, where nine dimension agents each walk every chunk: literal execution yields nine receipts or none. Territory ownership is a Step 3B idea. What both paths share is the uncoverable rule, and that needs no agent — a chunk is uncoverable iff its `maxLineChars` exceeds the read cap, which the orchestrator reads out of the plan before launching anything. That rule was also never threaded into Step 7, so a green PR with an unread chunk could receive a public LGTM. Any uncoverable chunk now downgrades APPROVE to COMMENT and must be named in the body. Also: the capture recipes redirected into `.qwen/tmp` before anything created it; a file-path review of an unchanged file produced an empty plan that no agent could read, and the skill now branches to a full-file read instead; and the docs classifier called `website/src/App.tsx` prose while calling `packages/cua-driver/docs/*.md` source — it now matches prose extensions under a documentation directory at any depth. * fix(review): tell agents what a severity means before asking for one The severity definitions lived once, in Step 6 — after every severity had already been assigned. Step 3's finding format asked each agent for `Severity: Critical | Suggestion | Nice to have` and never said what the words meant. The agents that fill that field are separate subagents with separate priors and no shared definition between them, so each fell back on its own, and the priors disagree. Observed on a live review of PR QwenLM#6635 — a run of the skill as it stands on main, whose Step 3 and Step 6 text this branch inherits unchanged. One review, CHANGES_REQUESTED, ten inline comments. Six were Critical, and four of those six were coverage gaps: "zero test coverage", "no references to `workers`", "no test exercises this". Two Suggestions in the same review were the identical class. The verdict is computed from Criticals alone, so that PR was blocked partly on the strength of findings its own reviewer had, elsewhere, called suggestions. The two genuine Criticals — a fail-fast that no longer fires before the daemon reports healthy, and a startup failure path that never closes the HTTP server — would have blocked it on their own. The definitions now sit in the finding format that every agent is handed, they are listed among the things every agent prompt must carry, and Step 6 points back at them rather than restating them. A missing test is a Suggestion: "this file has zero references to X" is a coverage statistic, not a defect. Two shapes stay Critical because something is genuinely wrong — a test asserting the opposite of the intended behaviour, and a test weakened or deleted in the diff so new behaviour passes. If a missing test would let a specific incorrect behaviour ship, report that behaviour and cite the gap as evidence. * fix(review): walk cross-file edges in both directions Cross-file impact analysis only ever asked "will the existing callers break?" Every bullet was about signature compatibility, and the budget rule told agents in so many words to "skip unchanged-signature modifications". A field added to an interface changes no signature and breaks no caller, so the analysis was blind to it by construction. The failure that exposed this, on PR QwenLM#6621: the diff added `deviceFlowRegistry?` to WorkspaceRuntime and passed it into the dispatcher for every secondary ACP mount, and nothing anywhere assigned it. The reviewing agent saw the declaration, found no writer, wrote "intentionally deferred to a later milestone", and filed a Suggestion to fix the JSDoc. The reader was AcpDispatcher — a file the diff never touched — where `if (!this.deviceFlowRegistry)` turned `auth/device_flow/start` into an INTERNAL_ERROR and `auth/status` into an empty list on every non-primary workspace. Workspace-qualified ACP shipped its authentication dead, and the review called it a documentation nit. A second reviewer filed the same observation as Critical; the author fixed it with code and dropped the field. Reading cannot find this. The declaration, the pass-through, and the read sit in three different places, and the read is outside the diff, so no agent reaches it by paging through hunks. Only a grep for the read sites does. So: for every field, option, or optional parameter the diff adds, grep its read sites, including outside the diff, and ask what happens when it arrives undefined. Severity is decided at the read site, not the declaration. And an agent must not explain an unpopulated field with author intent it cannot observe — "reserved for future use" is a claim about a person, not about code, and reaching for one means filling a hole in your own field of view. * fix(review): pin the diff base, and make the review body checkable Three defects, all found by reading what live reviews actually posted. The diff base. Agents were handed a diff command and left to choose a base. `main..HEAD` and `main...HEAD` differ by one character and by the entire meaning of the review: a two-dot diff against a main that has moved shows main's later commits reversed, so main's fixes read as the branch's regressions. A review of PR QwenLM#6626 approved the four files the PR actually changed, then warned the author publicly that their branch carried "typo regressions" in a file the PR never touched and should be rebased. main had corrected `compatability` to `compatibility` after the fork point. The branch had done nothing. Capture now resolves the base once and hands agents a file; they never see a ref name, and a finding in a file outside the report's `files[]` is not a finding about this PR. The review body. "A Suggestion never goes in body" is stated twice and was violated anyway, because a model holding a finding it cannot anchor would rather say it somewhere than drop it. On PR QwenLM#6631 an unanchorable Suggestion about `session.ts:2048` — a line in no hunk — became a second paragraph of the public review body. So the rule stops being prose: for COMMENT the body is exactly one of three sentences plus the footer and nothing else, and you read what you are about to send and confirm it. A Suggestion that will not anchor is deleted; it is already in the terminal output and the Step 8 report. The downgrade sentence. On PR QwenLM#6489 a review with three Suggestions and no Critical announced it had been "downgraded from Approve" — telling the author the PR would otherwise have been approved, which was false: a Suggestion-only review is COMMENT on its own. Decide the event from the findings first, apply the downgrade flag second, and write the sentence only if it changed the answer. * fix(review): decide the event by counting, not by weighing A review of PR QwenLM#6584 filed three inline Suggestions and submitted APPROVE with an empty body. GitHub recorded it as an approval. The rule it broke has been in Step 7 all along --- APPROVE means no Critical *and* no Suggestion --- and so has the one about the body, which is empty only for REQUEST_CHANGES. Both were stated twice. Both were ignored. They are ignored because at submit time the model is reasoning about what it wants to say, and "these are only suggestions, the PR is fine" is a sentence it can talk itself into. Nothing in that sentence is a count. So the event and the body become arithmetic. Count the Criticals, count the Suggestions, read the row off a three-row table, and only then apply the downgrade flags --- which can turn APPROVE or REQUEST_CHANGES into COMMENT and nothing else. Then read back what you are about to send and confirm it matches the row. A body holding text the table does not authorise is a finding that failed to anchor; if it is a Suggestion, it gets deleted, not relocated into public prose that no line of code answers to. This subsumes the body-only invariant added in the previous commit, which the same submit-time reasoning had already defeated once, on PR QwenLM#6631. * fix(review): stop the plan report outgrowing the read it must fit in The report tells an agent how to page everything else, so it has to be readable in one `read_file` — about 25 000 characters. Running the real `fetch-pr` against PR QwenLM#6457 produced 25 070. Two constraints pull against each other. Compact JSON is a single enormous line, and `read_file` pages at line boundaries, so a report too big for one call could never be read at all. Indented JSON pages fine but spends four lines on `{ "start": 812, "end": 815 }`, and a heavily rewritten file contributes hundreds of them: `QQChannel.ts` alone carries 140 added ranges and 49 hunks. So indent the structure and inline the leaves. Same JSON, same keys, one range per line, still pageable — and 28% smaller. The QwenLM#6457 report goes from 25 070 bytes to 18 042, and the "page it" warning that used to fire on a seven-file PR now stays quiet. The earlier attempt at this trimmed `addedRanges` to heavy files only and landed at 24 992 bytes on the same PR. Eight bytes of headroom was not a fix. Tests pin the three properties that matter: the collapsed text parses back to an identical object, no range spans two lines, and a path that literally spells a range is not mistaken for one — JSON escapes the quotes inside a string value, and the collapse patterns require unescaped ones. * fix(review): prune the worktree registration a deleted directory leaves behind `cleanStale` and `cleanup` both guarded `git worktree remove` behind `existsSync(path)`, and neither ever pruned. Delete the directory by hand — which is exactly what reclaiming disk with `rm -rf .qwen/tmp` does — and git keeps the worktree registered but missing. From then on `/review` on that PR cannot run: $ git worktree add .qwen/tmp/review-pr-6457 qwen-review/pr-6457 fatal: '...' is a missing but already registered worktree; use 'add -f' to override, or 'prune' or 'remove' to clear and the branch delete that `cleanStale` does next fails too, because the phantom worktree still has that branch checked out. Nothing in the review command surface ran `git worktree prune`, so nothing ever cleared it. This surfaced running the real skill: the orchestrator's first `fetch-pr` failed, it fell back to `qwen review cleanup`, and retried. The leak is not rare — three abandoned worktrees from May and June were still registered in this checkout, one per review that died before Step 9. `releaseWorktree` now does both halves in the order they depend on: remove the directory if it is there, prune the registration unconditionally (a no-op when nothing is stale), and only then let the caller delete the branch. Both callers share it. The tests drive real git. Deleting a worktree directory by hand and re-adding it throws "missing but already registered" without the prune, and `branch -D` throws "used by worktree" — both assertions fail if the prune is removed, which is the point of writing them. * fix(review): put the open comments where a truncated read will find them `read_file` returns the first `truncateToolOutputThreshold` characters — 25 000 by default — sets `isTruncated`, and pages by line. `pr-context` wrote "## Open inline comments (no replies yet — may still need attention)" last, so on a PR with a long history it was the first thing lost, and nothing read the flag that said so. On PR QwenLM#5738 that section began at character 27 125 of a 31 220-character file. The review submitted "Reviewed — no blockers." Five Critical threads were unresolved; four had in fact been addressed, but the fifth — `clearCiEnv()` clearing only `CI*` while `writeTerminalTitle` branches on `TMUX`/`STY`/ `ZELLIJ`/`DVTM` — was live, in the diff, and never seen. Regenerating the context for ten PRs: four lost part or all of the section, and all four were the PRs with the most review rounds. Small PRs never trip it. - Emit the open threads before the already-discussed ones. The findings a round must answer outrank the ones already settled. - `pr-context` warns when the file exceeds the threshold, naming any headings past the cut, and says so plainly when the loss is inside the last section's body instead. - Step 2 of SKILL.md now tells the agent to read `isTruncated` and page the remainder before Step 3. Reordering buys headroom; it does not create it. A 40 000-character context still loses its tail, which is what the warning is for. * fix(review): load this repo's review rules, and re-check open Criticals before approving Two gaps the dogfood on live PRs surfaced, both invisible from reading the skill. `load-rules` looks for a `## Code Review` heading in AGENTS.md and QWEN.md. Neither had one, so it wrote an empty file on every run: every `/review` in this repo reviewed with zero project rules. Add the section, distilled from the conventions already scattered through AGENTS.md (ESM, no cross-package relative imports, kebab-case/PascalCase naming, collocated tests, comments-only-when-why), plus the two hard lessons below. The section loads from the base branch by design — a PR cannot inject its own review rules — so it takes effect once merged. The skill treated a zero-Critical outcome as a fallback rather than a claim. On one PR it published two Criticals citing code not present at the reviewed commit (a fabricated blocker on an already-approved PR); on another it submitted C=0 while a live, twice-filed Critical still stood (a dropped blocker). Add a step before the verdict: for each unresolved Critical on the PR, read the code at the reviewed commit and record still-stands / fixed-by-this-diff / cannot-tell. The event follows from the code, not from the finding count or the thread flags — `isResolved`/`isOutdated` track the anchored line, not whether the bug was fixed. - AGENTS.md: new `## Code Review` section. - load-rules.ts: export `extractCodeReviewSection`; load-rules.test.ts covers the boundary scan and asserts AGENTS.md's own section extracts non-empty, so deleting the heading fails the build. - SKILL.md: re-verification step ahead of the Verdict.
Resolves conflicts in Session.ts (candidate-parts loop: combine finalRoundText accumulation with messageDisplay?.addChunk under the try/finally) and Session.test.ts (interleave the two MessageDisplay cron tests with main's preconditions/verdicts describe blocks).
|
@qwen-code /resolve |
|
Resolved the merge conflicts by hand (the bot couldn't push to this org-owned fork) and merged current |
|
Qwen Code resolved the merge conflicts, but could not push to Merge Conflict Resolution SummaryPR: #6489 — feat(hooks): add MessageDisplay hook for mid-turn streaming Conflicted files1.
|
|
|
||
| /** Send one payload through MessageBus; failures are logged, never thrown. */ | ||
| private dispatch(displayedText: string, isFinal: boolean): Promise<void> { | ||
| return this.messageBus |
There was a problem hiding this comment.
[Critical] messageBus.request() is called with only 2 arguments — the request payload and the response type. The AbortSignal is embedded inside the payload object (signal: this.signal) but never passed as the 4th argument to request(). The MessageBus.request() signature accepts signal?: AbortSignal as its 4th parameter and uses it to wire up an abort handler that rejects the promise and cleans up the response listener.
Without it, the default 60-second setTimeout inside request() fires without .unref() and without an abort listener. When drainWithTimeout() resolves at 5s and finish() returns, the underlying bus request keeps the Node.js event loop alive for up to 55 more seconds. In headless qwen -p mode, the process prints its output and then appears to hang. On abort (user Ctrl+C), in-flight bus requests are not cancelled through the bus's own abort mechanism — they rely solely on the handler-side signal and fall back to the 60s bus timeout.
| return this.messageBus | |
| return this.messageBus | |
| .request<HookExecutionRequest, HookExecutionResponse>( | |
| { | |
| type: MessageBusType.HOOK_EXECUTION_REQUEST, | |
| eventName: 'MessageDisplay', | |
| input: { | |
| message_id: this.messageId, | |
| displayed_text: displayedText, | |
| is_final: isFinal, | |
| }, | |
| signal: this.signal, | |
| }, | |
| MessageBusType.HOOK_EXECUTION_RESPONSE, | |
| MESSAGE_DISPLAY_DRAIN_TIMEOUT_MS, | |
| this.signal, | |
| ) |
Passing MESSAGE_DISPLAY_DRAIN_TIMEOUT_MS as the 3rd argument also bounds the bus request to the same drain budget instead of the default 60s.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /resolve |
|
Qwen Code resolved the merge conflicts, but could not push to Merge Conflict Resolution Summary — PR #6489Branch: Conflicts1.
|
Resolve conflicts from #6676 (drop isolated scheduled-task mode): - Session.ts #executeCronPromptInner: keep the cron loop's MessageDisplay dispatcher/addChunk (matching the ACP/notification/Stop loops); drop finalRoundText, whose verdict-capture mechanism main removed. - Session.test.ts: main deleted the entire 'isolated scheduled tasks' describe. Re-add the two in-session cron MessageDisplay tests adapted to main's scheduler idiom (plain { prompt } job; runMode/isolated is gone).
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No blockers found. Suggestion-level recommendations are in the Suggestion summary comment below.
— qwen3.7-max via Qwen Code /review
✅ Local verification report (maintainer)I built and exercised this PR locally as a merge reference. Verdict: the Environment — PR merge commit 1. Static gates + full test suiteRan the PR's 10 touched test files plus 2. Live end-to-end on the bundled CLIUnit tests aside, I drove the real Scenario A — fires repeatedly mid-turn, cumulative, before Scenario B — a tool-using turn produces two messages, each with its own id. The model streamed text, called Scenario C — a slow hook coalesces losslessly (O(1) backlog). Re-ran Scenario A with a Scope note (for transparency)
🇨🇳 中文版本(点击展开)✅ 本地验证报告(维护者)作为合并参考,我在本地构建并实际运行了本 PR。结论: 环境 —— PR 合并提交 1. 静态检查 + 全量测试运行了 PR 改动的 10 个测试文件,外加 2. 在打包 CLI 上的真实端到端验证除单元测试外,我用真实的
范围说明(如实告知)
Verification run on the bundled CLI with a local fake streaming model + a real command hook; screenshots are rendered from the actual hook logs. |










Summary
Adds a
MessageDisplayhook event — fires repeatedly as the assistant's replystreams, before
Stop(which only fires once at the end of the turn). Fixesthe gap described in #6488: today there's no way to observe a reply
incrementally in either the terminal UI or an ACP/IDE session;
Stopis theonly hook that sees the reply text, and it only fires once the whole turn is
done.
concept), fire-and-forget with no control effects — purely observational,
like
Notification/PostCompact.MessageDisplayDispatcher(
packages/core/src/core/message-display-dispatcher.ts), one per modelcall, wired into every raw streaming loop that can produce assistant text:
the shared
for awaitloop inclient.ts, and all four raw-stream loopsin
Session.ts(main prompt, Stop-hook-forced continuation, cron tick,background notification) for the ACP/IDE/
qwen servepath. Earlierrevisions of this PR assumed
client.ts's loop was shared by both paths —it isn't; that turned out to be wrong and needed the second insertion
point in
Session.ts.delivery plus one pending payload per
message_id. A newer flushoverwrites the pending payload losslessly (
displayed_textiscumulative), and
is_finalis dispatched immediately — even alongside astill-running stale delivery — so it's never stuck behind a queue and
always precedes
Stop.MESSAGE_DISPLAY_DRAIN_TIMEOUT_MS(5s, sharedacross all
finish()calls on the same dispatcher) for the final deliveryto complete, then proceeds; a slower hook keeps running in the background.
Text is debounced (~200ms) for mid-stream firings.
Design notes / open questions for reviewers
displayed_textis cumulative, not a delta — this removed an entire classof reassembly bugs for hook authors and for the dispatcher's own
coalescing logic.
MESSAGE_DISPLAY_DEBOUNCE_MS = 200) and the draintimeout (
MESSAGE_DISPLAY_DRAIN_TIMEOUT_MS = 5000) are constants, notconfigurable — open to making either configurable if that's wanted, kept
it simple for a first pass.
message_idis minted fresh per streaming call (perclient.tssendMessageStreaminvocation, or perSession.tsraw-stream loop) —each is its own "message" from a display/narration standpoint.
is_finalhasalready been dispatched, its outcome is moot and no longer warned on;
its completion order relative to the final delivery is otherwise
unspecified — stateful consumers should treat
is_finalas terminal permessage_id, not as "arrives after all other deliveries have settled."Size
This PR is larger than the ~2000-changed-line guideline in CONTRIBUTING.md
(currently ~2200 lines across 28 files). It grew past that threshold over
three rounds of review driven by @wenshao's local A/B verification against
real terminal UI /
qwen serve/ headless runs, each round fixing aconcrete correctness gap the previous round's design had (the ACP path
never firing at all, an unbounded slow-hook backlog, a dropped
is_finalon headless exit, then a residual 2x drain ceiling once that fix shipped).
Splitting the dispatcher rework out from the original single-loop insertion
would leave an intermediate PR in the same broken state one of these rounds
found and fixed — I think it's better reviewed as the one change it ended
up being than as a sequence of PRs each reintroducing a bug the next one
patches. Happy to reconsider if a maintainer would rather see it split.
Test plan
npm run preflight(lint, format, full test suite, build)message-display-buffer.test.ts),dispatcher coalescing/drain logic (
message-display-dispatcher.test.ts),hook-system wiring (
hookEventHandler/hookSystem/hookPlanner/hookAggregatortest suites), theconfig.tsbridge's fieldextraction,
client.ts's streaming-loop integration(
client.test.ts), andSession.ts's four raw-stream loops(
Session.test.ts).docs/users/features/hooks.mddelivery-semantics section for the payloadshape and firing/drain guarantees instead.
Fixes #6488